-
Notifications
You must be signed in to change notification settings - Fork 5
feat: Add max negative caps and enforce ledger bounds #618
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughThis pull request introduces Changes
Sequence DiagramsequenceDiagram
actor User as Administrator
participant UI as currency-detail.tsx
participant API as CurrencyController
participant Service as CurrencyService
participant Ledger as LedgerService
participant DB as Database
User->>UI: Set max negative balance value
UI->>API: PATCH /api/currencies/:id/max-negative {value}
API->>Service: updateMaxNegativeBalance(id, value, admin)
Note over Service: Validate:<br/>- Currency exists<br/>- Admin permission<br/>- allowNegative=true<br/>- Value is null or ≤0
Service->>DB: Update currency.maxNegativeBalance
DB-->>Service: Updated Currency
Service-->>API: Currency entity
API-->>UI: {id, maxNegativeBalance, ...}
UI->>UI: Invalidate cache<br/>(currency, accountDetails)
Note over User,Ledger: Later: User attempts transfer
User->>API: POST /transfer {amount, ...}
API->>Ledger: transfer(currencyId, amount, ...)
Note over Ledger: Compute balance change<br/>if (maxNegativeBalance set<br/>AND balance+amount < limit)<br/>throw error
alt Balance within limit
Ledger->>DB: addLedgerEntry(...)
DB-->>Ledger: Ledger entry
Ledger-->>API: Success
else Balance exceeds limit
Ledger-->>API: Insufficient funds error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
platforms/eCurrency-api/src/services/LedgerService.ts (1)
96-124: Wrap the transfer method in a database transaction to prevent concurrent balance validation violations.There is a time-of-check to time-of-use (TOCTOU) gap between fetching
currentBalance(line 96) and persisting ledger entries (lines 112-139). Concurrent transfers from the same account could both pass validation before either commits, potentially violating themaxNegativeBalanceconstraint.Use
AppDataSource.transaction()with proper isolation level (similar to the pattern in GroupService.ts), or implement row-level locking withFOR UPDATEto ensure balance validation and ledger creation are atomic.
🧹 Nitpick comments (4)
platforms/eCurrency-api/src/database/entities/Currency.ts (1)
39-40: TypeORM decimal columns return strings at runtime.TypeORM's
decimaltype returns values as strings, not JavaScriptnumbers. The TypeScript typenumber | nullis misleading and may cause subtle bugs when performing arithmetic or comparisons without explicit conversion.Consider adjusting the type or using a transformer:
- @Column("decimal", { precision: 18, scale: 2, nullable: true }) - maxNegativeBalance!: number | null; + @Column("decimal", { precision: 18, scale: 2, nullable: true, transformer: { + to: (value: number | null) => value, + from: (value: string | null) => value === null ? null : parseFloat(value) + }}) + maxNegativeBalance!: number | null;Alternatively, update the type to
string | nulland convert explicitly at usage sites.platforms/eCurrency-api/src/controllers/CurrencyController.ts (1)
162-166: Consider adding semantic validation formaxNegativeBalance.The current validation only checks that the value is a valid number (or null). Given the semantic meaning (a floor for how negative a balance can go), consider validating that non-null values are
<= 0:// Allow null to clear; otherwise must be a number const parsedValue = value === null || value === undefined ? null : Number(value); if (parsedValue !== null && Number.isNaN(parsedValue)) { return res.status(400).json({ error: "Invalid value for maxNegativeBalance" }); } + if (parsedValue !== null && parsedValue > 0) { + return res.status(400).json({ error: "maxNegativeBalance must be zero or negative" }); + }platforms/eCurrency-api/src/services/CurrencyService.ts (1)
20-47: AligncreateCurrencymaxNegativeBalance invariants with update path
createCurrencynow accepts and persistsmaxNegativeBalancebut does not enforce the same constraints you added inupdateMaxNegativeBalance(must only be set whenallowNegativeis true, must benullor<= 0, notNaN). That means callers can create a currency in a state that the update path would later reject (e.g.allowNegative = falsewith a non‑null cap, or a positive cap).To keep invariants consistent and avoid surprising states, consider normalizing and validating here as well, e.g.:
// Generate eName (UUID with @ prefix) const ename = `@${uuidv4()}`; + const normalizedMaxNegativeBalance = + allowNegative && maxNegativeBalance !== null ? maxNegativeBalance : null; + + if (normalizedMaxNegativeBalance !== null) { + if (Number.isNaN(normalizedMaxNegativeBalance)) { + throw new Error("Invalid max negative balance value"); + } + if (normalizedMaxNegativeBalance > 0) { + throw new Error("Max negative balance must be zero or negative"); + } + } + const currency = this.currencyRepository.create({ name, description, ename, groupId, createdBy, allowNegative, - maxNegativeBalance, + maxNegativeBalance: normalizedMaxNegativeBalance, });platforms/eCurrency/client/src/pages/currency-detail.tsx (1)
292-301: Optional: Format maxNegativeBalance consistently with other monetary valuesHere you display
maxNegativeBalancewith a baretoLocaleString(), while balance and total supply are formatted with two decimal places. For consistency and readability (especially since the DB column is decimal with scale), consider aligning the formatting:- {currency.allowNegative - ? (currency.maxNegativeBalance !== null && currency.maxNegativeBalance !== undefined - ? Number(currency.maxNegativeBalance).toLocaleString() - : "No cap") - : "Not applicable"} + {currency.allowNegative + ? (currency.maxNegativeBalance !== null && currency.maxNegativeBalance !== undefined + ? Number(currency.maxNegativeBalance).toLocaleString(undefined, { + minimumFractionDigits: 2, + maximumFractionDigits: 2, + }) + : "No cap") + : "Not applicable"}Not required for correctness, but it keeps the UI consistent with the way balances and total supply are shown.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
platforms/eCurrency-api/src/controllers/CurrencyController.ts(7 hunks)platforms/eCurrency-api/src/database/entities/Currency.ts(1 hunks)platforms/eCurrency-api/src/database/migrations/1765784749012-migration.ts(1 hunks)platforms/eCurrency-api/src/index.ts(1 hunks)platforms/eCurrency-api/src/services/CurrencyService.ts(3 hunks)platforms/eCurrency-api/src/services/LedgerService.ts(3 hunks)platforms/eCurrency/client/src/pages/currency-detail.tsx(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
platforms/eCurrency-api/src/database/migrations/1765784749012-migration.ts (1)
platforms/eCurrency-api/src/database/migrations/1765039149572-migration.ts (1)
Migration1765039149572(3-14)
platforms/eCurrency-api/src/index.ts (1)
platforms/eCurrency-api/src/middleware/auth.ts (1)
authGuard(36-41)
platforms/eCurrency/client/src/pages/currency-detail.tsx (1)
platforms/eCurrency/client/src/lib/apiClient.ts (1)
apiClient(5-10)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (8)
platforms/eCurrency-api/src/database/migrations/1765784749012-migration.ts (1)
1-14: LGTM!The migration correctly adds and removes the
maxNegativeBalancecolumn with the appropriate type (numeric(18,2)), matching the entity definition and following the established migration pattern in the codebase.platforms/eCurrency-api/src/services/LedgerService.ts (2)
43-49: LGTM!The
existingBalanceparameter is a sensible optimization that avoids a redundant database query when the caller has already fetched the balance for validation purposes.
103-108: Consider validatingmaxNegativeBalanceis non-positive.The current logic assumes
maxNegativeBalancerepresents a floor (e.g.,-100). If a positive value is accidentally set, the conditionnewBalance < maxNegativeBalancewould never trigger for typical debits leading to negative balances. Consider adding validation in the controller/service to ensuremaxNegativeBalance <= 0when set.platforms/eCurrency-api/src/index.ts (1)
115-115: LGTM!The new PATCH endpoint follows the existing route patterns, uses appropriate HTTP semantics for a partial update, and is correctly protected with
authGuard.platforms/eCurrency-api/src/controllers/CurrencyController.ts (2)
18-18: LGTM!The
maxNegativeBalancefield is correctly extracted from the request body, passed to the service, and included in the response. The implementation follows the existing patterns for other currency fields.Also applies to: 29-29, 40-40
174-185: LGTM!The response structure is consistent with other currency endpoints, including all relevant fields.
platforms/eCurrency-api/src/services/CurrencyService.ts (1)
87-117: Max negative balance update flow and validation look solidThe new
updateMaxNegativeBalancemethod correctly:
- Verifies the currency exists and the requester is a group admin.
- Disallows setting a cap when
allowNegativeis false.- Ensures the value is either
nullor a non‑NaN number that is<= 0.This keeps the persisted state consistent with the business rules around negative balances.
platforms/eCurrency/client/src/pages/currency-detail.tsx (1)
1-1: State and effect wiring for max negative balance looks consistentThe added React Query client usage,
MAX_NEGATIVE_SLIDERconstant, andmaxNegative*state plus theuseEffectthat derives the absolute input fromcurrency.maxNegativeBalanceform a coherent flow and ensure the UI is initialized from the latest server value when the currency data loads or is refetched. No functional issues spotted here.Also applies to: 18-18, 25-28, 56-64
Description of change
Issue Number
closes #553
closes #554
closes #555
Type of change
How the change has been tested
Change checklist
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.